Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start adding a new rerun order #2067

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Start adding a new rerun order #2067

wants to merge 6 commits into from

Conversation

aslakhellesoy
Copy link
Contributor

@aslakhellesoy aslakhellesoy commented Jun 28, 2022

🤔 What's changed?

This adds a new rerun order that can be used with the --order option.

⚡️ What's your motivation?

We are building a test case prioritization tool that can predict the tests that are most likely to fail. Our tool generates a rerun.txt file. Users can then use this order to run tests in the predicted order, increasing the probability of seeing test failures earlier in the test run.

Currently, specifying a rerun.txt file will filter scenarios, but the order in which they are run is still dictated by the --order option (defaulting to defined).

The proposed rerun order will use the contents of the rerun.txt file as the desired order to run tests in.

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@IMalaniak IMalaniak marked this pull request as ready for review June 28, 2022 10:38
@coveralls
Copy link

coveralls commented Jun 28, 2022

Coverage Status

Coverage decreased (-0.08%) to 98.223% when pulling cb69d4f on add-rerun-order into 4dac0a7 on main.

src/api/types.ts Outdated Show resolved Hide resolved
@@ -34,13 +36,15 @@ interface IParseGherkinMessageStreamRequest {
* @param eventDataCollector
* @param gherkinMessageStream
* @param order
* @param unexpandedFeaturePaths
* @param pickleFilter
*/
export async function parseGherkinMessageStream({
Copy link
Contributor

@davidjgoss davidjgoss Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is deprecated and will be removed in a future major release, however in the meantime it'd be better to avoid making a breaking change to it. Could we make the new field optional?

(It's fine to change orderPickles, that's an internal one.)

@davidjgoss davidjgoss mentioned this pull request Jul 1, 2022
@charlierudolph
Copy link
Member

I'm confused why this is needed. I would think if order is "defined", the order of the tests as defined in the rerun file should be respected.

Can you better outline what the current case is? Is the current order based only at the file level and not at the pickle level?

I don't think this should be a new order and just feels like a more proper implementation of the "defined" order (ie the order of file / file & line references passed)

@aslakhellesoy
Copy link
Contributor Author

The original purpose of the rerun file is to rerun just the failing scenarios.

Users might still want to run them in random order. Or perhaps they want to run them in the defined order (alphabetically ordered files, scenarios top-to-bottom).

Filtering and ordering are two orthogonal concepts, so I think they should be kept separate.

That said, I wouldn’t object to defaulting to rerun order when a rerun file is used, as long as users can still choose a different order if they wish.

@charlierudolph
Copy link
Member

charlierudolph commented Jul 2, 2022

Rerun still feels like a bad name to me for the name of this order. We have defined a rerun format as being one where each line is a number of file / line combinations that point to file / scenarios / scenario outlines. But you could also provide that in the exact same way as a number of CLI arguments.

The current "defined" isn't very exact. I think its currently the following:

  • If given no cli arguments -> loads features directory run, directory is expanded to have files in alpha order, runs each file top to bottom
  • If given cli arguments that are files / file + lines -> order files by first appearance of each file, filtered scenarios top to bottom

Examples

./bin/cucumber-js features/profiles.feature features/named_hooks.feature
> Runs features/profiles.feature - top to bottom
> Runs features/named_hooks.feature - top to bottom
./bin/cucumber-js features/profiles.feature:31 features/named_hooks.feature features/profiles.feature:40
> Runs features/profiles.feature:31
> Runs features/profiles.feature:40
> Runs features/named_hooks.feature 

The second example seems wrong to me as ideally we'd run what the input was. If we fixed that, it would give you the exact order you have implemented as rerun

@charlierudolph
Copy link
Member

Okay, after looking at trying to implement the fix I described, believe that isn't worth it. I still don't think we should tie the name of this to "rerun". I think my ideal would be something like:

  • feature - runs by feature load order top to bottom (same as current "defined" which we deprecate)
  • scenario - what you are implementing, requires all arguments to point directly to scenarios
  • random - same as is

Thoughts? Note, I'm also happy to help with these changes

`Cannot use rerun order because ${pathB} was not in the rerun order. Did you forget to specify @rerun.txt?`
)
}
return indexA - indexB
Copy link
Member

@charlierudolph charlierudolph Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I think this could be something like

const ordered = new Array(unexpandedFeaturePaths.length);
picklesWithDocument.forEach(item => {
  // get index in unexpandedFeaturePaths, throw if not found
  ordered[index] = item
})

// remove empty indexes (only occurs if have duplicates in list and we just use the first) 

That should be O(n) instead O(nlogn)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants